Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pr/jonenst/86 #91

Merged
merged 5 commits into from
Jul 22, 2021
Merged

Pr/jonenst/86 #91

merged 5 commits into from
Jul 22, 2021

Conversation

Fuzzyma
Copy link
Member

@Fuzzyma Fuzzyma commented Jul 21, 2021

#86

@Fuzzyma Fuzzyma mentioned this pull request Jul 21, 2021
Merged
@jonenst
Copy link
Contributor

jonenst commented Jul 21, 2021

Hi @Fuzzyma , I just uploaded your code (your last commit b60a2a3) to https://codepen.io/jonenst/pen/mdrgwoJ?editors=1000 and the first test case doesn't work (it should always stop panning when the green square goes out of the screen. it doesn't work on the right side in the first case). The commit before that (b432e49) worked.

@jonenst
Copy link
Contributor

jonenst commented Jul 21, 2021

Here's b432e49: https://codepen.io/jonenst/pen/yLbzyoQ

@jonenst
Copy link
Contributor

jonenst commented Jul 21, 2021

restoring the signs on the offsets fixes some bugs but the are still problems:

@@ -62,18 +62,18 @@ extend(Svg, {
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX) {
-              viewportLeftOffset = offset / 2
-              viewportRightOffset = -offset / 2
+              viewportLeftOffset = -offset / 2
+              viewportRightOffset = offset / 2
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX) {
-              viewportRightOffset = -offset
+              viewportRightOffset = offset
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
-              viewportLeftOffset = offset
+              viewportLeftOffset = -offset
             }
           } else {
             if (

@jonenst
Copy link
Contributor

jonenst commented Jul 21, 2021

The following diff fixes everything I think (deployed here: https://codepen.io/jonenst/pen/dyWVPLv?editors=1001 ):

  • the sign is not symettric
  • the ratio is not symmetric
@@ -56,42 +56,48 @@ extend(Svg, {
           const changedAxis = svgAspectRatio > viewboxAspectRatio ? 'width' : 'height'
           const isWidth = changedAxis === 'width'
 
-          const offset = box[changedAxis] / viewboxAspectRatio * svgAspectRatio - box[changedAxis]
-          if ((isMeet && isWidth) || (!isMeet && !isWidth)) {
+          const changeHorizontal = (isMeet && isWidth) || (!isMeet && !isWidth)
+          const aspectRatioRatio = changeHorizontal ? svgAspectRatio / viewboxAspectRatio
+            : viewboxAspectRatio / svgAspectRatio
+          const offset = isMeet
+            ? box[changedAxis] * aspectRatioRatio - box[changedAxis]
+            : box[changedAxis] - box[changedAxis] * aspectRatioRatio
+          const sign = isMeet ? 1 : -1
+          if (changeHorizontal) {
             if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX) {
-              viewportLeftOffset = offset / 2
-              viewportRightOffset = -offset / 2
+              viewportLeftOffset = sign * -offset / 2
+              viewportRightOffset = sign * offset / 2
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX) {
-              viewportRightOffset = -offset
+              viewportRightOffset = sign * offset
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
-              viewportLeftOffset = offset
+              viewportLeftOffset = sign * -offset
             }
           } else {
             if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMID) {
-              viewportTopOffset = -offset / 2
-              viewportBottomOffset = offset / 2
+              viewportTopOffset = sign * -offset / 2
+              viewportBottomOffset = sign * offset / 2
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN) {
-              viewportBottomOffset = offset
+              viewportBottomOffset = sign * offset
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
-              viewportTopOffset = -offset
+              viewportTopOffset = sign * -offset
             }
           }

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Jul 21, 2021

Ohhh, I didnt realize that the offset calculation was flipped around (the svgAspectRatio / viewboxAspectRatio part).

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Jul 21, 2021

@jonenst I think this time I got it right. I don't think the sign thing is necessary because it cancels out at all times. I tested and found no issues but you are the expert here :)

@jonenst
Copy link
Contributor

jonenst commented Jul 21, 2021

582ecda still doesn't work (see https://codepen.io/pen/?editors=1000 on foo4) BUT you're right the sign always cancels out, and you only need to invert the signs in the else block and I think it's good ! https://codepen.io/jonenst/pen/yLbzaKG

           const offset = box[changedAxis] - box[changedAxis] * ratio
           if (changeHorizontal) {
             if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX) {
               viewportLeftOffset = offset / 2
               viewportRightOffset = -offset / 2
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX) {
               viewportRightOffset = -offset
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
               viewportLeftOffset = offset
             }
           } else {
             if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMID ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMID) {
-              viewportTopOffset = -offset / 2
-              viewportBottomOffset = offset / 2
+              viewportTopOffset = offset / 2
+              viewportBottomOffset = -offset / 2
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN) {
-              viewportBottomOffset = offset
+              viewportBottomOffset = -offset
             } else if (
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX ||
               preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
-              viewportTopOffset = -offset
+              viewportTopOffset = offset
             }
           }
 
         }
       }

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Jul 21, 2021

Wow I can stare at this for hours and wouldnt see it... thx!

@jonenst
Copy link
Contributor

jonenst commented Jul 22, 2021

cool I think you can merge this now !

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Jul 22, 2021

Awesome. Thanks for your help!

@Fuzzyma Fuzzyma merged commit 8d93ec5 into master Jul 22, 2021
@Fuzzyma Fuzzyma deleted the pr/jonenst/86 branch July 22, 2021 12:48
@jonenst
Copy link
Contributor

jonenst commented Jul 24, 2021

Thank you for this awesome lib :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Fuzzyma @jonenst